Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move the experimental frontends into a separate crate, so that when not using them they don't take several minutes to compile (and indirect dependencies). #168

Merged
merged 1 commit into from
Oct 19, 2024

Conversation

arnaucube
Copy link
Collaborator

@arnaucube arnaucube commented Oct 14, 2024

Move the experimental frontends into a separate crate, so that when not using them they don't take several minutes to compile (and indirect dependencies).

This saves several minutes (and MBs of data) on compilation time both when running tests in this repo, but also when using the sonobe lib as a dependency in external repos.

This solution was suggested by @ed255 when talking about this.

@arnaucube arnaucube force-pushed the creatify-experimental-frontends branch 5 times, most recently from 9bdacb5 to 27ab0e2 Compare October 14, 2024 16:19
Copy link

@pmikolajczyk41 pmikolajczyk41 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please accept apologies for interrupting, but have you considered hiding each frontend behind a dedicated feature (and making deps optional)?

Edit: I see you have :) can you share the reasons to abandon features in favour of crates?

@arnaucube
Copy link
Collaborator Author

arnaucube commented Oct 14, 2024

Indeed, that's a good point. This was initially done at #141 but the issue is that even if a frontend was 'not used' by not using its feature, it's dependencies were still being imported (and the dependencies of the dependencies, taking long minutes and MBs of data), when running tests but also when using Sonobe as external dependency.

With the separation into a different subpackage for the experimental frontends done in this PR, the users of the lib that don't use any of the experimental frontends will not get impacted by those dependencies that are in fact not used in that case, and only in the cases where the experimental frontends are specifically used those dependencies will get used.

As a side note, the recommended frontend is using arkworks (directly implementing the FCircuit trait), as the other experimental frontends add overhead when generating the witness which makes the folding step slower.

@pmikolajczyk41
Copy link

oh, okay; now I remember similar issue I had with private repo dependency in a workspace causing problems for every member crate, even for those, that didn't use it (even indirectly); afair a workaround it was to use some dummy patching, but in Sonobe case I totally understand and support your final decision - thanks!

Copy link
Member

@CPerezz CPerezz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

README.md Outdated Show resolved Hide resolved
folding-schemes/src/frontend/utils.rs Outdated Show resolved Hide resolved
frontends/Cargo.toml Show resolved Hide resolved
@arnaucube arnaucube force-pushed the creatify-experimental-frontends branch 2 times, most recently from 2bb66c1 to b42999f Compare October 16, 2024 10:27
Copy link
Collaborator

@dmpierre dmpierre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One suggestion regarding the README.md of the frontend crate. Otherwise lgtm!

frontends/README.md Outdated Show resolved Hide resolved
@arnaucube
Copy link
Collaborator Author

Will wait till #169 is merged, and then will rebase this PR (#168) to main (once it includes #169) and deal with the git conflicts in this branch (#168).

@arnaucube arnaucube force-pushed the creatify-experimental-frontends branch from b42999f to bff2c55 Compare October 18, 2024 09:50
@CPerezz
Copy link
Member

CPerezz commented Oct 18, 2024

Let's just merge this @arnaucube as I want to wait for Janabel to confirm that the PR works prev to merge it.

@arnaucube arnaucube marked this pull request as draft October 18, 2024 11:22
@arnaucube arnaucube force-pushed the creatify-experimental-frontends branch 6 times, most recently from 92b744e to fe3f84a Compare October 18, 2024 15:31
@arnaucube arnaucube marked this pull request as ready for review October 18, 2024 15:31
@arnaucube arnaucube force-pushed the creatify-experimental-frontends branch 3 times, most recently from c902429 to 0a9526d Compare October 18, 2024 15:40
@arnaucube arnaucube force-pushed the creatify-experimental-frontends branch 4 times, most recently from 49a871a to 8f01d87 Compare October 19, 2024 14:56
…ot using them they don't take several minutes to compile (and indirect dependencies).

This saves several minutes (and MBs of data) on compilation time both
when running tests in this repo, but also when using the sonobe lib as a
dependency in external repos.
@arnaucube arnaucube force-pushed the creatify-experimental-frontends branch from 8f01d87 to 43aa99e Compare October 19, 2024 16:14
@arnaucube arnaucube added this pull request to the merge queue Oct 19, 2024
Merged via the queue into main with commit 234600b Oct 19, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants